fix:draft report zero state improvements #3363
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/images/zero-states/invoice.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/app/fyle/my-view-report/my-view-report.page.html(1 hunks)src/app/fyle/my-view-report/my-view-report.page.scss(2 hunks)
| &--footer-conatiner { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| padding: 16px; | ||
| gap: 8px; | ||
| position: fixed; | ||
| bottom: 0; | ||
| width: 100%; | ||
| background-color: $pure-white; | ||
| box-shadow: 0 -2px 12px $pink-shadow; | ||
| z-index: 999; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Let's optimize the footer container styles, macha!
The footer container styling needs some improvements:
- Using
z-index: 999is like using a cannon to kill a mosquito! A lower value would suffice. - Fixed positioning with 100% width might cause issues on smaller screens.
&--footer-conatiner {
display: flex;
justify-content: space-between;
padding: 16px;
gap: 8px;
position: fixed;
bottom: 0;
width: 100%;
background-color: $pure-white;
box-shadow: 0 -2px 12px $pink-shadow;
- z-index: 999;
+ z-index: 10;
+ max-width: 100vw;
+ box-sizing: border-box;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &--footer-conatiner { | |
| display: flex; | |
| justify-content: space-between; | |
| padding: 16px; | |
| gap: 8px; | |
| position: fixed; | |
| bottom: 0; | |
| width: 100%; | |
| background-color: $pure-white; | |
| box-shadow: 0 -2px 12px $pink-shadow; | |
| z-index: 999; | |
| } | |
| &--footer-conatiner { | |
| display: flex; | |
| justify-content: space-between; | |
| padding: 16px; | |
| gap: 8px; | |
| position: fixed; | |
| bottom: 0; | |
| width: 100%; | |
| background-color: $pure-white; | |
| box-shadow: 0 -2px 12px $pink-shadow; | |
| z-index: 10; | |
| max-width: 100vw; | |
| box-sizing: border-box; | |
| } |
| &--zero-state-semibold-header { | ||
| font-size: 14px; | ||
| display: block; | ||
| color: $blue-black; | ||
| line-height: 1.5; | ||
| font-weight: 400; | ||
| } | ||
|
|
||
| &--zero-state-image-container { | ||
| height: 150px; | ||
| width: 150px; | ||
| overflow: hidden; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style pandratha style-a pandrom! (Let's style it with style!)
The zero state image container needs to be more responsive:
&--zero-state-image-container {
- height: 150px;
- width: 150px;
+ height: min(150px, 30vh);
+ width: min(150px, 90%);
overflow: hidden;
+ margin: 0 auto;
+ display: flex;
+ align-items: center;
+ justify-content: center;
+
+ img {
+ max-width: 100%;
+ height: auto;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &--zero-state-semibold-header { | |
| font-size: 14px; | |
| display: block; | |
| color: $blue-black; | |
| line-height: 1.5; | |
| font-weight: 400; | |
| } | |
| &--zero-state-image-container { | |
| height: 150px; | |
| width: 150px; | |
| overflow: hidden; | |
| } | |
| &--zero-state-semibold-header { | |
| font-size: 14px; | |
| display: block; | |
| color: $blue-black; | |
| line-height: 1.5; | |
| font-weight: 400; | |
| } | |
| &--zero-state-image-container { | |
| height: min(150px, 30vh); | |
| width: min(150px, 90%); | |
| overflow: hidden; | |
| margin: 0 auto; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| img { | |
| max-width: 100%; | |
| height: auto; | |
| } | |
| } |
| <div class="view-reports--footer-conatiner"> | ||
| <button | ||
| *ngIf="unreportedExpenses && unreportedExpenses.length > 0" | ||
| class="view-reports--unreported-cta" | ||
| class="btn-primary" | ||
| (click)="showAddExpensesToReportModal()" | ||
| > | ||
| Add complete expenses | ||
| Add complete expense | ||
| </button> | ||
| <button | ||
| class="btn-outline-secondary" | ||
| [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary': unreportedExpenses && unreportedExpenses.length > 0}" | ||
| (click)="addExpense()" | ||
| > | ||
| Add new expense |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thalaiva style button improvements needed!
The button section needs some improvements:
- The button text "Add complete expense" is not very clear. Consider "Add existing expense" instead.
- The ngClass condition can be simplified.
- <button
- class="btn-outline-secondary"
- [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary': unreportedExpenses && unreportedExpenses.length > 0}"
- (click)="addExpense()"
- >
- Add new expense
- </button>
+ <button
+ [class]="unreportedExpenses?.length ? 'btn-outline-secondary' : 'btn-primary'"
+ (click)="addExpense()"
+ >
+ Create new expense
+ </button>Also, consider extracting the unreportedExpenses check to a getter in the component:
get hasUnreportedExpenses(): boolean {
return this.unreportedExpenses?.length > 0;
}| <div class="view-reports--zero-state-image-container"> | ||
| <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" /> | ||
| </div> | ||
| <div class="view-reports--zero-state-semibold-header">Looks like you’ve got</div> | ||
| <div class="view-reports--zero-state-header">0 expense right now</div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali da! The zero state looks good, but let's make it perfect!
The alt text could be more descriptive for better accessibility.
- <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" />
+ <img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="view-reports--zero-state-image-container"> | |
| <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" /> | |
| </div> | |
| <div class="view-reports--zero-state-semibold-header">Looks like you’ve got</div> | |
| <div class="view-reports--zero-state-header">0 expense right now</div> | |
| </div> | |
| <div class="view-reports--zero-state-image-container"> | |
| <img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" /> | |
| </div> | |
| <div class="view-reports--zero-state-semibold-header">Looks like you've got</div> | |
| <div class="view-reports--zero-state-header">0 expense right now</div> | |
| </div> |
|
|
||
| &--zero-state-image-container { | ||
| height: 150px; | ||
| width: 150px; |
There was a problem hiding this comment.
would this work for all resolutions of mobile devices?
There was a problem hiding this comment.
@arjunaj5 I have checked it across multiple dimensions, and I think it looks good. At a dimension of 320px, it appears fine as well. Attaching a photo for reference.
There was a problem hiding this comment.
@Sishhhh, let's share this screenshot with the design team mentioning the resolution.
I think we will need more padding.
There was a problem hiding this comment.
@Dimple16 here is the updated ui :
Screen.Recording.2025-01-06.at.12.36.53.PM.mov
| const addExpenseButton = fixture.debugElement.nativeElement.querySelector( | ||
| '.view-reports--footer-conatiner .btn-outline-secondary' | ||
| ) as HTMLElement; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind-blowing suggestion: Use data-testid for more stable test selectors!
Hey there! Like a perfectly choreographed action sequence, your tests should be robust and reliable! Instead of relying on complex class-based selectors that can break when styles change, let's add a data-testid attribute to make our test selector more stable, thalaiva style!
- const addExpenseButton = fixture.debugElement.nativeElement.querySelector(
- '.view-reports--footer-conatiner .btn-outline-secondary'
- ) as HTMLElement;
+ const addExpenseButton = fixture.debugElement.nativeElement.querySelector(
+ '[data-testid="add-expense-button"]'
+ ) as HTMLElement;Don't forget to add the corresponding data-testid attribute to your component template:
<button data-testid="add-expense-button" class="view-reports--footer-conatiner btn-outline-secondary">
Add Expense
</button>
|
|
||
| &--zero-state-image-container { | ||
| height: 150px; | ||
| width: 150px; |
There was a problem hiding this comment.
@Sishhhh, let's share this screenshot with the design team mentioning the resolution.
I think we will need more padding.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/fyle/my-view-report/my-view-report.page.html(1 hunks)src/app/fyle/my-view-report/my-view-report.page.scss(2 hunks)
🔇 Additional comments (4)
src/app/fyle/my-view-report/my-view-report.page.scss (2)
59-71: Mind it! The footer container needs style optimization, macha!The footer container styling needs some improvements for better maintainability and responsiveness:
- Using
z-index: 999is overkill! A lower value would suffice.- Fixed positioning with 100% width might cause issues on smaller screens.
Apply this diff to optimize the styles:
&--footer-container { display: flex; justify-content: space-between; padding: 12px 16px; gap: 8px; text-wrap: nowrap; position: fixed; bottom: 0; width: 100%; background-color: $pure-white; box-shadow: 0 -2px 12px $pink-shadow; - z-index: 999; + z-index: 10; + max-width: 100vw; + box-sizing: border-box; }
374-386: Superstar says: Make the zero state more responsive!The zero state image container needs to be more responsive:
Apply this improvement:
&--zero-state-image-container { - height: 150px; - width: 150px; + height: min(150px, 30vh); + width: min(150px, 90%); overflow: hidden; + margin: 0 auto; + display: flex; + align-items: center; + justify-content: center; + + img { + max-width: 100%; + height: auto; + } }src/app/fyle/my-view-report/my-view-report.page.html (2)
173-178: Kabali da! Let's make the zero state perfect!The alt text could be more descriptive for better accessibility.
Apply this improvement:
- <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" /> + <img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" />
179-192: Thalaiva style button improvements needed!The button section needs some improvements:
Apply these changes:
- <button - id="add-expense" - [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary view-reports--cta': unreportedExpenses && unreportedExpenses.length > 0}" - (click)="addExpense()" - > - Add new expense - </button> + <button + id="add-expense" + [class]="unreportedExpenses?.length ? 'btn-outline-secondary view-reports--cta' : 'btn-primary'" + (click)="addExpense()" + > + Create new expense + </button>Also, consider extracting the unreportedExpenses check to a getter in the component:
get hasUnreportedExpenses(): boolean { return this.unreportedExpenses?.length > 0; }
| &--cta { | ||
| font-size: 14px !important; | ||
| @media (max-width: 350px) { | ||
| font-size: 12px !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style pandratha style-a pandrom! (Let's style it with style!)
The CTA styles could be improved by:
- Avoiding
!importantdeclarations - Using CSS custom properties for better maintainability
Consider this improvement:
&--cta {
- font-size: 14px !important;
+ font-size: var(--cta-font-size, 14px);
@media (max-width: 350px) {
- font-size: 12px !important;
+ --cta-font-size: 12px;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &--cta { | |
| font-size: 14px !important; | |
| @media (max-width: 350px) { | |
| font-size: 12px !important; | |
| } | |
| } | |
| &--cta { | |
| font-size: var(--cta-font-size, 14px); | |
| @media (max-width: 350px) { | |
| --cta-font-size: 12px; | |
| } | |
| } |
| } | ||
|
|
||
| &--cta { | ||
| font-size: 14px !important; |
There was a problem hiding this comment.
the design specifically requires a font size of 14px for this element. However, there is an existing global SCSS rule that sets font-size: 16px !important, removing it could potentially break the design in other parts of the application where the global rule is applied. so i had to override the global rule using 14px !important.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/app/fyle/my-view-report/my-view-report.page.html(1 hunks)src/app/fyle/my-view-report/my-view-report.page.scss(2 hunks)src/app/fyle/my-view-report/my-view-report.page.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (5)
src/app/fyle/my-view-report/my-view-report.page.spec.ts (1)
935-935: Superstar style test improvement, macha!Using ID selector
#add-expenseinstead of class selector makes the test more robust and reliable.src/app/fyle/my-view-report/my-view-report.page.scss (3)
396-400: 🧹 Nitpick (assertive)Make it responsive like a boss!
The zero state image container needs responsive dimensions for better adaptability across different screen sizes.
Apply this diff to make the image container responsive:
&--zero-state-image-container { - height: 150px; - width: 150px; + height: min(150px, 30vh); + width: min(150px, 90%); overflow: hidden; + margin: 0 auto; + display: flex; + align-items: center; + justify-content: center; + + img { + max-width: 100%; + height: auto; + } }Likely invalid or redundant comment.
73-78: 🧹 Nitpick (assertive)Mind it! Let's avoid !important, it's not our style!
Using !important makes styles harder to maintain and override when needed.
Consider using CSS custom properties for better maintainability:
&--cta { - font-size: 14px !important; + font-size: var(--cta-font-size, 14px); @media (max-width: 350px) { - font-size: 12px !important; + --cta-font-size: 12px; } }Likely invalid or redundant comment.
59-71: 🧹 Nitpick (assertive)Style pandratha style-a pandrom! (Let's style it with style!)
The footer container styling needs optimization:
- z-index: 999 is unnecessarily high
- Fixed positioning with 100% width needs constraints for smaller screens
Apply this diff to improve the footer styles:
&--footer-container { display: flex; justify-content: space-between; padding: 12px 16px; gap: 8px; text-wrap: nowrap; position: fixed; bottom: 0; width: 100%; background-color: $pure-white; box-shadow: 0 -2px 12px $pink-shadow; - z-index: 999; + z-index: 10; + max-width: 100vw; + box-sizing: border-box; }Likely invalid or redundant comment.
src/app/fyle/my-view-report/my-view-report.page.html (1)
175-180: 🧹 Nitpick (assertive)Enhance accessibility like a superstar!
The alt text could be more descriptive for better screen reader experience.
Apply this diff to improve accessibility:
- <img src="../../../assets/images/zero-states/invoice.png" alt="No expense in this report" /> + <img src="../../../assets/images/zero-states/invoice.png" alt="Empty report state - No expenses have been added to this report yet" />Likely invalid or redundant comment.
| class="btn-primary view-reports--cta" | ||
| (click)="showAddExpensesToReportModal()" | ||
| > | ||
| Add complete expenses | ||
| Add complete expense | ||
| </button> | ||
| <button | ||
| id="add-expense" | ||
| [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary view-reports--cta': unreportedExpenses && unreportedExpenses.length > 0}" | ||
| (click)="addExpense()" | ||
| > | ||
| Add new expense |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the button logic, make it punch dialogues style!
The button implementation needs improvements:
- "Add complete expense" is not very clear
- Complex ngClass condition can be simplified
Apply these changes to improve clarity and maintainability:
- <button
- class="btn-primary view-reports--cta"
- (click)="showAddExpensesToReportModal()"
- >
- Add complete expense
- </button>
+ <button
+ class="btn-primary view-reports--cta"
+ (click)="showAddExpensesToReportModal()"
+ >
+ Add existing expense
+ </button>
- <button
- id="add-expense"
- [ngClass]="{'btn-primary': !(unreportedExpenses && unreportedExpenses.length > 0), 'btn-outline-secondary view-reports--cta': unreportedExpenses && unreportedExpenses.length > 0}"
- (click)="addExpense()"
- >
- Add new expense
- </button>
+ <button
+ id="add-expense"
+ [class]="unreportedExpenses?.length ? 'btn-outline-secondary view-reports--cta' : 'btn-primary'"
+ (click)="addExpense()"
+ >
+ Create new expense
+ </button>Also, consider adding this getter to the component for cleaner template logic:
get hasUnreportedExpenses(): boolean {
return this.unreportedExpenses?.length > 0;
}
|
Clickup
https://app.clickup.com/t/86cx8fb73
Code Coverage
Please add code coverage here
UI Preview
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests